Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix write_incr #559

Merged
merged 23 commits into from
Apr 14, 2023
Merged

Conversation

jderber-NOAA
Copy link
Contributor

@jderber-NOAA jderber-NOAA commented Apr 13, 2023

Fixes #558

@@ -202,7 +211,7 @@ subroutine write_fv3_inc_ (grd,sp_a,filename,mype_out,gfs_bundle,ibin)
iqg = getindex(svars3d,'qg')

istatus=0
call gsi_bundlegetpointer(gfs_bundle,'q', sub_qanl, iret); istatus=istatus+iret
call gsi_bundlegetpointer(svalinc(ibin2),'q', sub_qanl, iret); istatus=istatus+iret
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch on this, no idea why this one is different from the rest... does this have any implcations for current GFSv16?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jderber-NOAA I am a little confused here. In the following around line 313 (in this program), there is
! compute q increment sub_q = sub_qanl(:,:,:) - ges_q1(:,:,:,ibin)
So, seems to me, the original code intentionally gets analysis q in full at the above (line 205) .
I am wondering why the original treatment is used . However, if the change of line 214 is used, the following line 313 should also be changed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree good catch. I do not understand why this variable is handled differently. But the intent of this change is not to change the science, but to make the code work. The line is reverted to use gfs_bundle and the tests resubmitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All regression tests passed and test of code was fine. I believe this is ready for re-review.

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not test but changes look straightforward and correct to me

@RussTreadon-NOAA
Copy link
Contributor

As per the 4/10/2023 email sent to the GSI subversion email list, PRs must be merged within 6 weeks of creation. PRs not merged within the 6 week window will be returned to the originating developer (see GSI Wiki).

This PR was opened 4/13/2023. The due date for this PR is set to 5/25/2023, six weeks after 4/13/2023.

@RussTreadon-NOAA
Copy link
Contributor

All regression tests pass. Two peer reviews approve. I'll reach out to the Handling Review Team for final approval and merge.

Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve.

@jderber-NOAA
Copy link
Contributor Author

Thanks Cory and Ting! Up to Russ now!

@jderber-NOAA
Copy link
Contributor Author

I seem to be a few minutes (or less) behind everyone this morning!

@TingLei-NOAA
Copy link
Contributor

@jderber-NOAA A following up question , the previous assignment between gsi bundle (like currently line 195) while without defining the operator overload by "use gsi_bundlemod, only: assignment(=)" added in this PR, would it cause any actual impacts? Namely, will this PR create different increment (correct) fields in the generated increment files? I am not sure if current regression tests compare the generated increment files.

@jderber-NOAA
Copy link
Contributor Author

Ting, That is a good question. I don't know. For my test case, the code would not complete without these changes so I could not compare. Could you do a comparison? If there is a difference, I am certain that the new is the more correct one.

@TingLei-NOAA
Copy link
Contributor

@jderber-NOAA I am not sure if current regression tests for global gsi will create the inc files.
I will talk to global DA developers off line for help on this. I will give an update as soon as possible.

@RussTreadon-NOAA
Copy link
Contributor

Merger into develop on hold pending feedback from @TingLei-NOAA

@TingLei-NOAA
Copy link
Contributor

An update: Thanks to @RussTreadon-NOAA 's information, I directly compared the increment files in the regression test : global_3dvar on hera: in /scratch1/NCEPDEV/stmp2/Ting.Lei/GSI/tmpreg_global_3dvar, I ran
^

diff -h global_3dvar_hiproc_contrl/siginc.nc global_3dvar_hiproc_updat/siginc.nc
V
There are none of output. So, I conclude this means they are identical .
I am having some issues on dogwood (the wcoss2 machine available to me now) for git . So I haven't run tests on it.

@RussTreadon-NOAA
Copy link
Contributor

Given @TingLei-NOAA confirmation of identical increments go ahead and merge this PR into develop

@RussTreadon-NOAA RussTreadon-NOAA merged commit 1661c15 into NOAA-EMC:develop Apr 14, 2023
TingLei-daprediction pushed a commit to TingLei-daprediction/GSI that referenced this pull request May 23, 2023
TingLei-daprediction pushed a commit to TingLei-daprediction/GSI that referenced this pull request May 23, 2023
TingLei-daprediction pushed a commit to TingLei-daprediction/GSI that referenced this pull request May 24, 2023
TingLei-daprediction pushed a commit to TingLei-daprediction/GSI that referenced this pull request May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with write_incr
4 participants